-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor NO_TICKET Various improvements #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: cursor_combined_20260121_qodo_grep_cursor_copilot_1_base_refactor_no_ticket_various_improvements_pr228
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| navigationActionStack.arrangedSubviews + | ||
| leadingPageActionStack.arrangedSubviews + | ||
| pageActionStack.arrangedSubviews | ||
| browserActionStack.arrangedSubviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate browserActionStack causes missing alpha animation
Medium Severity
The stacks variable includes browserActionStack.arrangedSubviews twice but omits trailingPageActionStack.arrangedSubviews. This appears to be a copy-paste error during the rename from pageActionStack to trailingPageActionStack. As a result, the trailing page action buttons won't have their alpha set to 1.0 during toolbar animations, leaving them invisible after animation completes.
|
|
||
| // Page action spacing | ||
| let hasPageActions = !pageActionStack.arrangedSubviews.isEmpty | ||
| let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong stack checked for divider width calculation
Medium Severity
The hasPageActions check was changed from the renamed trailingPageActionStack to leadingPageActionStack. The locationDividerView is positioned between locationView and trailingPageActionStack, so the divider width should be set based on whether there are trailing page actions, not leading ones. This may cause the divider to show/hide incorrectly.
| insertSubview(rightSkeletonAddressBar, aboveSubview: toolbar) | ||
|
|
||
| toolbar.leadingAnchor.constraint(equalTo: leftSkeletonAddressBar.trailingAnchor).isActive = true | ||
| toolbar.leadingAnchor.constraint(equalTo: rightSkeletonAddressBar.trailingAnchor).isActive = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toolbar leading constraint uses wrong skeleton anchor
High Severity
The toolbar's leadingAnchor constraint was changed from leftSkeletonAddressBar.trailingAnchor to rightSkeletonAddressBar.trailingAnchor. Since the right skeleton is positioned outside the container's trailing edge, this constraint places the toolbar's leading edge beyond the right skeleton, creating an impossible or broken layout with the toolbar potentially having zero or negative width.
Benchmark PR from qodo-benchmark#228
Note
Improves testability and clarifies toolbar layout while adjusting constraints.
Browser.statusBarOverlay,Browser.headerContainer,Browser.overKeyboardContainer,Browser.bottomContainer,Browser.bottomContentStackView, andBrowser.contentContainer, and assigns them inBrowserViewController.pageActionStacktotrailingPageActionStackand updates all related constraints/usages. Adjusts page-action divider width logic to depend onleadingPageActionStackpresence. Keeps stacks hidden based onscrollAlpha.shoudShowKeyboard→shouldShowKeyboardand updates Redux dispatch condition. Tweaks toolbar/skeleton address bar constraints and simplifies skeleton layout setup when swiping tabs is enabled.Written by Cursor Bugbot for commit ce7cd22. Configure here.